Skip to content

fix(cli): propagate delegated Go child exit codes through finalizers#5841

Merged
Coly010 merged 1 commit into
developfrom
columferry/cli-1879-legacy-shell-child-exit-code-delegated-path-semantics
Jul 9, 2026
Merged

fix(cli): propagate delegated Go child exit codes through finalizers#5841
Coly010 merged 1 commit into
developfrom
columferry/cli-1879-legacy-shell-child-exit-code-delegated-path-semantics

Conversation

@Coly010

@Coly010 Coly010 commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

What kind of change does this PR introduce?

Bug fix — legacy-shell child exit-code and delegated-path semantics.

What is the current behavior?

Fixes CLI-1879.

Three related gaps around Go-delegated subprocess paths in the legacy shell:

  1. LegacyGoProxy.exec/execCapture (shared/legacy/go-proxy.layer.ts) called ProcessControl.exit() directly on a non-zero child exit or "binary not found". That's a real process.exit() — it skips every Effect.ensuring finalizer between the call site and runCli (telemetry flush, command instrumentation), and loses the child's exact exit code (collapsing everything to whatever process.exit() was called with, with no chance for runCli's own exit-code logic to run).
  2. Because execCapture's non-zero-exit branch hard-exited the process, it never reached withJsonErrorHandling, so a --output-format json/stream-json db reset --experimental failure emitted no structured error envelope at all — the process just died mid-flight.
  3. db reset --experimental --linked (no resolved version) unconditionally resolved the linked DB connection — including minting/verifying a temporary Postgres login role over the Management API — before checking whether the remaining flow delegates to the Go child. On that branch the resolved connection was never used: the delegated Go child re-runs its own ParseDatabaseConfig (and mints its own temp role) once it starts, so the TS-side mint was pure duplicate privileged work.

(A fourth item from the same issue — forwarding --linked=false's target selector verbatim to the delegated child — was already correctly implemented and already covered by a passing test; no code change was needed there.)

What is the new behavior?

  • New LegacyGoChildExitError (shared/legacy/legacy-go-child-exit.error.ts) carries a spawned child's exact exit code via Effect's Runtime.errorExitCode marker. LegacyGoProxy.exec/execCapture and the hidden db __db-bootstrap seam now fail with this typed error instead of calling ProcessControl.exit(), so the failure flows through the normal Effect channel: finalizers run first, then runCli/withJsonErrorHandling exit with the child's real code — in every output format, including a Ctrl-C mid-recreate (e.g. exit 130) instead of a generic 1.
  • runCli's handledProgram special-cases LegacyGoChildExitError (by concrete type) to skip its own generic output.fail stderr line, since the child already printed its own detailed failure to the inherited stderr and Go itself never prints a second line on top of that. This is deliberately not keyed on Effect's shared Runtime.errorReported marker — CliError.ShowHelp also sets that marker, and gating on it would have silently suppressed the Go-parity Error: required flag(s) "..." not set message for every missing-required-flag error. (Caught by architect review before merge; regression test added.)
  • withJsonErrorHandling now reads Runtime.getErrorExitCode instead of hardcoding 1 when setting the process exit code, so the exact code propagates under json/stream-json too, not just text mode.
  • db reset --experimental --linked now checks whether it's about to delegate to the Go child before calling LegacyDbConfigResolver.resolve(), skipping the redundant temp-role mint on that path. The linked-project-cache finalizer is unaffected (it already reads a separately pre-loaded ref, exactly so this case still works).
  • Updated SIDE_EFFECTS.md's exit-code tables for db reset/db start to reflect the child's exact code, and touched-up a couple of stale doc comments describing the old process.exit()-based behavior.

Deliberately left open (judgement calls, not blockers)

  • The JSON error envelope for a delegated linked+experimental connection/mint failure is now a generic "supabase-go exited with code N (see stderr for details)" rather than the specific TS error the old (duplicate-minting) code path used to surface. This is an accepted tradeoff: the real detail is on the inherited stderr, and Go itself has no JSON error-envelope concept to hold this to a parity standard against.
  • The bootstrap seam's JSON error code field changes from LegacyDbBootstrapError to LegacyGoChildExitError for a non-zero child exit specifically. Nothing in this codebase treats that field as a stable public contract (most Legacy*Error tags already leak their raw class name into it), so this wasn't treated as a compatibility break.
  • The sibling db __shadow seam (legacy-pgdelta.seam.layer.ts) intentionally keeps its own generic domain error rather than adopting LegacyGoChildExitError — its failure is a TS-authored summary over noisy docker/pgdelta stderr (not a passthrough of a real user-facing Go child), and Go itself collapses shadow-DB failures to a generic exit 1. Left a comment in place explaining the divergence so a future reader doesn't "fix" it into inconsistency.

Test plan

New/updated coverage in go-proxy.layer.unit.test.ts, run.unit.test.ts, json-error-handling.unit.test.ts, reset.integration.test.ts, and start.integration.test.ts — exact exit-code propagation, finalizers running after a non-zero exit, the JSON error envelope, the skipped pre-delegation resolve (and that the sibling --db-url delegate path still resolves), and a regression guard for the ShowHelp/MissingOption suppression bug caught during review.

…(CLI-1879)

LegacyGoProxy.exec/execCapture and the hidden db __db-bootstrap seam called
ProcessControl.exit() directly on a non-zero child exit, which skipped
Effect.ensuring finalizers (telemetry flush, instrumentation) and, for
execCapture, bypassed withJsonErrorHandling entirely so json/stream-json
delegated failures emitted no structured error envelope.

Route both through a new LegacyGoChildExitError instead: it carries the
child's exact exit code via Runtime.errorExitCode (read by both runCli and
withJsonErrorHandling) so finalizers run first and the real code still
reaches the user in every output format, and runCli special-cases it (by
concrete type, not Effect's shared Runtime.errorReported marker, which
CliError.ShowHelp also sets and would otherwise suppress the Go-parity
"required flag(s) not set" message) to skip a duplicate generic stderr line
the child already printed itself.

Also stop db reset --experimental --linked from minting a temporary
Management API login role before delegating to the Go child, which mints its
own and made the TS wrapper's mint pure duplicate work.
@Coly010

Coly010 commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Already looking forward to the next diff.

Reviewed commit: 306dd14fd7

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@Coly010 Coly010 self-assigned this Jul 9, 2026
@Coly010 Coly010 marked this pull request as ready for review July 9, 2026 12:18
@Coly010 Coly010 requested a review from a team as a code owner July 9, 2026 12:18
@github-actions

github-actions Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Supabase CLI preview

npx --yes https://pkg.pr.new/supabase/cli/supabase@306dd14fd76d9f271ba63287aea83d0569f6ffb7

Preview package for commit 306dd14.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

["db", "reset"],

P2 Badge Keep the global signal handler for native reset paths

This opt-out matches every db reset invocation, but only the local/delegated branches install their own holdSignals listener. For native remote resets such as supabase db reset --linked --version ... or non-experimental remote resets, reset.handler.ts never delegates to the Go child, so disabling signalAwareProgram leaves SIGINT to Bun/Node's default immediate exit. In that scenario the reset handler's finalizers (linked-project cache and telemetry flush) are skipped, regressing the finalizer guarantee this change is trying to preserve; the opt-out needs to be limited to the child-owning path or the native path needs its own signal handling.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@Coly010 Coly010 added this pull request to the merge queue Jul 9, 2026
Merged via the queue into develop with commit 7bc8457 Jul 9, 2026
34 checks passed
@Coly010 Coly010 deleted the columferry/cli-1879-legacy-shell-child-exit-code-delegated-path-semantics branch July 9, 2026 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants